-
Notifications
You must be signed in to change notification settings - Fork 34
[interop] Add Support for Anonymous Declarations #434
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@_i1.JS() | ||
external _AnonymousUnion_1189263 get responseObject; | ||
@_i1.JS() | ||
external _AnonymousConstructor_1059824 get productConstr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I may have miscommunicated earlier what I meant, but I was thinking of making these constructor values like
Product productConstr(
num id,
String name,
num price,
) =>
Product(
id: id.toDouble(),
name: name,
price: price.toDouble(),
);
instead. I'm not really sure it's useful for users to have this constructor as a getter (and it avoids the extra anonymous extension type if we don't), but I'm also totally okay with going the current route if you feel like it's better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The constructor call shouldn't be a getter, it should be an override on
call
, so it is intended to be called asproductConstr()
. - I do not know the implications of having it as a variable vs as a function.
- I feel the anonymous extensions are useful, as in this example, they can be extended/implemented and typealiased.
Do you have any thoughts concerning these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the anonymous extensions are useful, as in this example, they can be extended/implemented and typealiased.
I doubt users will want or care about these but I suppose they don't really hurt. It just adds more anonymous types that make it harder to distinguish between them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The extending/implementing I meant was code-side (i.e in the .d.ts
file).
It just adds more anonymous types that make it harder to distinguish between them.
This is true. I assume most people wanting to use these types anyways would have a typedef
on them anyways. In either case, if a typedef
is placed, it'd be useful to have it as a type to typedef as, wouldn't it?
web_generator/test/integration/interop_gen/ts_typing_input.d.ts
Outdated
Show resolved
Hide resolved
}); | ||
|
||
test('Sub Type Interface Test', () { | ||
final a = InterfaceDeclaration( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd consider a case where we check the LCA of three types where the pairwise LCAs are different than the LCA of all the types. This is to check we're not accidentally composing the LCAs (which we aren't).
e.g.
C -> B -> A
E -> D -> B
F -> H -> B
E -> G -> A
F -> G -> A
LCA{E, F} = G but LCA{C, E, F} (B) != LCA{C, LCA{E, F}} (A)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Nike! Just a couple more comments.
|
||
@override | ||
set isNullable(bool isNullable) {} | ||
bool isNullable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not make this late
and only set it when we have the value instead of defaulting like we do in line 32?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, but it isn't guaranteed to be set. It is only set if isNullable
is set
/// isNullable can be null
Type _transformType(TSTypeNode type,
{bool parameter = false, bool typeArg = false, bool? isNullable}) {
// code
}
@@ -51,7 +46,7 @@ class BuiltinType extends Type { | |||
.where((p) => typeParams.length != 1 || p != $voidType) | |||
.map((p) => p.emit(options))) | |||
..url = fromDartJSInterop ? 'dart:js_interop' : null | |||
..isNullable = _isNullable ?? options?.nullable); | |||
..isNullable = isNullable || (options?.nullable ?? false)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, isNullable
should have been set so we should not see any late initialization errors with the propose change above.
Sorry, GitHub UI is atrocious and didn't show me all the unresolved comments in the commits tab so I have a few more. :) |
Fixes #385
Fixes #422
Fixes #410
Fixes #433
This PR adds support for generating declarations for anonymous declarations, such as anonymous objects, unions, closures and constructor types.
This PR also adds support for nullable types (types unioned with
undefined
and/ornull
).A hashing function is used for consistently hashing string objects, which is used for hashing identifiers for anonymous unions, objects and more, as well as comparing such names to allow reusing of such types.
string | number | boolean
may look like the following:JSTupleX
declaration generated depending on the number of members it may have (i.e if a 2 member tuple exists, it will be an instance ofJSTuple2<A, B>
). The tuple extension type looks like the following:call
. While Anonymous function types ((str: string) => void
) have theircall
implementations asexternal
, anonymous constructor types (new (str: string) => T
to constructT
) are implemented as function calls to create the given object. In the future after Make JSExportedDartFunction generic and add a toJSGeneric function sdk#54557, we should be able to replace this with a genericJSFunction
.